Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Splitting the hardware intrinsic tests into more test groupings #61973

Merged
merged 2 commits into from
Nov 24, 2021

Conversation

tannergooding
Copy link
Member

This should help mitigate #60154 by ensuring that each test group is "sufficiently small". It should also help with the overall test throughput.

The split is roughly on "ISA" but could be split as more fine-grained if required.

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Nov 23, 2021
@ghost
Copy link

ghost commented Nov 23, 2021

Tagging subscribers to this area: @JulieLeeMSFT
See info in area-owners.md if you want to be subscribed.

Issue Details

This should help mitigate #60154 by ensuring that each test group is "sufficiently small". It should also help with the overall test throughput.

The split is roughly on "ISA" but could be split as more fine-grained if required.

Author: tannergooding
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@tannergooding
Copy link
Member Author

CC. @jkotas, @echesakovMSFT

@jkotas
Copy link
Member

jkotas commented Nov 23, 2021

We will need to figure out how to fix these tests into the larger test refactoring that @trylek and @jkoritzinsky are working on. @trylek and @jkoritzinsky Do you have thoughts on this?

Having said that, I think this change is a fine short-term fix for the GC stress timeout problem.

@trylek
Copy link
Member

trylek commented Nov 23, 2021

My current thinking is that, according to Jeremy's initial implementation, the "new" group projects will be "intentional" in the sense that the grouping won't be automatic, we'll use structure of the test tree and performance of the individual test groups to decide where to put those. The grouping projects will ultimately replace the Helix grouping project and will be used to construct directly runnable Helix work items (I'm working on this new logic right now). Thus, once the conversion has been completed, we should be able to delete both the legacy xUnit wrappers and the testgrouping MSBuild script.

@echesakov
Copy link
Contributor

I was thinking that maybe we need two different partitionings for these tests - one for GCStress pipelines (where tests for each API method - e.g. Arm.AdvSimd.Add would get a separate test project - similar to what I tried to do in https://github.com/echesakovMSFT/runtime/tree/Arm64-Split-And-Group-HardwareIntrinsics-Arm-Tests-By-Method-Name) and another partition for non-GCStress pipelines (similar to what we currently have).

The reasoning for that is there is no perfect partitioning that fits all test scenarios. The finer partitioning would make running the tests in innerloop pipelines much slower, while the coarser one would almost always fail in GCStress pipelines.

@tannergooding
Copy link
Member Author

I was thinking that maybe we need two different partitionings for these tests

The goal here is to just have this as a short-term workaround until the larger test refactoring that @trylek and @jkoritzinsky are working on. The larger refactoring should take care of the requirements around appropriately parallelizing and handling timeouts, etc.

This PR however should expand things just enough to get CI to stop failing until that happens as the new failures are largely only due to the xplat tests added for Vector64/128/256 and them all running as part of the same group without this change.

@jkotas
Copy link
Member

jkotas commented Nov 24, 2021

I was thinking that maybe we need two different partitionings for these tests - one for GCStress pipelines

I would hope that we can avoid that by simply applying a test timeout multiplier for GCStress legs.

Copy link
Contributor

@echesakov echesakov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The goal here is to just have this as a short-term workaround

@tannergooding I should've been more specific that I was not suggesting to implement what I described in this PR. I was simply describing an option.

As a workaround the changes LGTM.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants